Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't leave eval via next #17

Closed
wants to merge 1 commit into from

Conversation

dominikschulz
Copy link
Contributor

Eval is not a loop and as such should not be left
using loop control statements.

This commit replaces those next statements with return
statements. The control flow should be exactly the same
since the enclosing loop will move on to the next iteration
after a successfull return from the eval.

@dominikschulz
Copy link
Contributor Author

Actually the build didn't fail ... travis failed.

@abh
Copy link
Contributor

abh commented Oct 7, 2012

Why return 1 instead of just return?

@dominikschulz
Copy link
Contributor Author

Usually "return 1" is used to indicate that something was successful. In this case it is used to indicate that the option was successfully processed. We could use a bare "return" instead, but that could possible lead to confusion in the future.

I've seen plenty of examples where a little more verbosity would have prevented serious bugs, but I'd be fine with a bare return, too.

Please advise.

@dominikschulz
Copy link
Contributor Author

Ok, so bare return it is. I'll update the PR ASAP.

Eval is not a loop and as such should not be left
using loop control statements.

This commit replaces those next statements with return
statements. The control flow should be exactly the same
since the enclosing loop will move on to the next iteration
after a successfull return from the eval.
@dominikschulz
Copy link
Contributor Author

Using bare returns as per request and rebased to latest HEAD.

@yannk
Copy link
Member

yannk commented Oct 13, 2012

done

@yannk yannk closed this Oct 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants